Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix peak activity example & bugs in peak_activity #2130

Merged
merged 4 commits into from
Oct 25, 2023

Conversation

zm711
Copy link
Collaborator

@zm711 zm711 commented Oct 25, 2023

The final function in the example was missing the peaks argument causing it to fail. (See link)

https://spikeinterface.readthedocs.io/en/latest/modules_gallery/widgets/plot_4_peaks_gallery.html#sphx-glr-modules-gallery-widgets-plot-4-peaks-gallery-py

I also added keyword arguments to encourage their use and make clear what arguments are going into the examples.

@zm711 zm711 added the documentation Improvements or additions to documentation label Oct 25, 2023
@zm711
Copy link
Collaborator Author

zm711 commented Oct 25, 2023

Also very minor, but the example uses British English spelling rather than US spelling. It honestly doesn't matter much, but in your function names you always use US spelling, so do you want me to make sure everything I see is consistent in the future or do you not care?

@zm711 zm711 added bug Something isn't working widgets Related to widgets module labels Oct 25, 2023
@zm711
Copy link
Collaborator Author

zm711 commented Oct 25, 2023

Also discovered a couple bugs in _plot_one_bin().

  1. It was trying to pull the recording from self, when rec is an argument for the function.
  2. The positional arguments were out of order, so I switched to keyword to prevent this error

I'll see if I find anything else while I read through it all again.

@zm711 zm711 changed the title Fix peak activity example Fix peak activity example & bugs in peak_activity Oct 25, 2023
@zm711
Copy link
Collaborator Author

zm711 commented Oct 25, 2023

Based on my reading getting the dynamic content will be more trouble than it's worth in this PR. This is good as is for now (unless you want the dynamic example deleted since it appears static anyway).

@samuelgarcia
Copy link
Member

oups. My bad. I did not check examples when I ported theses widgets into new API.
Thnaks for the fix.

@samuelgarcia samuelgarcia merged commit 2956c27 into SpikeInterface:main Oct 25, 2023
9 checks passed
@zm711
Copy link
Collaborator Author

zm711 commented Oct 25, 2023

I tested the dynamic example locally and it looked super super cool @samuelgarcia!

@zm711 zm711 deleted the fix-peak-activity branch October 25, 2023 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation widgets Related to widgets module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants